-
Notifications
You must be signed in to change notification settings - Fork 35.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
deploy: remove some tools when cross-compiling for macOS #29890
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
INSTALL_NAME_TOOL=$(INSTALL_NAME_TOOL) OTOOL=$(OTOOL) STRIP=$(STRIP) $(PYTHON) $(OSX_DEPLOY_SCRIPT) $(OSX_APP) $(OSX_VOLNAME) -translations-dir=$(QT_TRANSLATION_DIR) | ||
OTOOL=$(OTOOL) $(PYTHON) $(OSX_DEPLOY_SCRIPT) $(OSX_APP) $(OSX_VOLNAME) -translations-dir=$(QT_TRANSLATION_DIR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bitcoin/contrib/macdeploy/macdeployqtplus
Line 212 in c7567d9
installnametoolbin=os.getenv("INSTALL_NAME_TOOL", "install_name_tool") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bitcoin/contrib/macdeploy/macdeployqtplus
Line 231 in c7567d9
stripbin=os.getenv("STRIP", "strip") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you read the description? Neither of these code paths are hit when cross compiling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you read the description?
I did.
Neither of these code paths are hit when cross compiling.
It is not obvious from reading the macdeployqtplus
code. Maybe add a few comments to make it clear?
This could only be called in code paths that cannot be hit.
This is only needed when compiling on macOS. This means we can also better scope the usage of `-headerpad_max_install_names`.
If we aren't using install_name_tool when cross-compiling, we don't need to test for / add it to LDFLAGS when that is the case.
Added a commit to decrease the scope of using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tested that both the guix build and local make deploy
still work on Intel macOS 14.4.1.
751ede1b4f680d44c97a9aab396e0a485e3a47c88ecc30ec8b83e53784fc3f50 guix-build-1a9aa8d4eedf/output/arm64-apple-darwin/SHA256SUMS.part
871cf387d5d60efc0ec9e50f975a9f44b2e2f9b7d92d1f2744affc3c8a5e1655 guix-build-1a9aa8d4eedf/output/arm64-apple-darwin/bitcoin-1a9aa8d4eedf-arm64-apple-darwin-unsigned.tar.gz
6a8de4ac9647549d146a53a9167b00f72ac09168939284b76fa9b6cf81595fea guix-build-1a9aa8d4eedf/output/arm64-apple-darwin/bitcoin-1a9aa8d4eedf-arm64-apple-darwin-unsigned.zip
6a00443340b2b8d13ed86a5d3947845ec2da20cfb0f0f80cce1434a0a4581557 guix-build-1a9aa8d4eedf/output/arm64-apple-darwin/bitcoin-1a9aa8d4eedf-arm64-apple-darwin.tar.gz
ec0320586c07bcc6b80505ed6b3de6307fd99d43b505919bb4f95ff27f1d8991 guix-build-1a9aa8d4eedf/output/dist-archive/bitcoin-1a9aa8d4eedf.tar.gz
14a0907bf9c91a2ca19b5a9b53071159be36d82d69c287ffd6a621695f4a637d guix-build-1a9aa8d4eedf/output/x86_64-apple-darwin/SHA256SUMS.part
862b59e90473c37d4f95d083fccf12416ddc92f9177a2ffb71180a11b3432d9b guix-build-1a9aa8d4eedf/output/x86_64-apple-darwin/bitcoin-1a9aa8d4eedf-x86_64-apple-darwin-unsigned.tar.gz
ddcb16882dde50880f9dffc6ac7ab5ac01b220bbedad4c16e9ff15dcf3b30cef guix-build-1a9aa8d4eedf/output/x86_64-apple-darwin/bitcoin-1a9aa8d4eedf-x86_64-apple-darwin-unsigned.zip
720a0689c916e878a5ff050b6183e993af751950b906593d4cbaf4b22a22466c guix-build-1a9aa8d4eedf/output/x86_64-apple-darwin/bitcoin-1a9aa8d4eedf-x86_64-apple-darwin.tar.gz
Makefile.am
Outdated
@@ -126,7 +126,7 @@ $(OSX_ZIP): deploydir | |||
cd $(APP_DIST_DIR) && find . | sort | $(ZIP) -X@ $@ | |||
|
|||
$(APP_DIST_DIR)/$(OSX_APP)/Contents/MacOS/Bitcoin-Qt: $(OSX_APP_BUILT) $(OSX_PACKAGING) | |||
INSTALL_NAME_TOOL=$(INSTALL_NAME_TOOL) OTOOL=$(OTOOL) STRIP=$(STRIP) $(PYTHON) $(OSX_DEPLOY_SCRIPT) $(OSX_APP) $(OSX_VOLNAME) -translations-dir=$(QT_TRANSLATION_DIR) | |||
INSTALL_NAME_TOOL=$(INSTALL_NAME_TOOL) OTOOL=$(OTOOL) $(PYTHON) $(OSX_DEPLOY_SCRIPT) $(OSX_APP) $(OSX_VOLNAME) -translations-dir=$(QT_TRANSLATION_DIR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
78b6b5c: note for other reviewers
BUILD_OS
is set to darwin
when either (see configure.ac
):
- we're not cross-compiling and
TARGET_OS=darwin
; or - we are cross-compiling and
$build_os=darwin
BUILD_DARWIN
is set when [test "$BUILD_OS" = "darwin"]
. So the !BUILD_DARWIN
code path here is taken when we are cross compiling (from a non-darwin system).
After this commit $STRIP
is only used for Windows stuff.
It's not clear to me what $STRIP
is/was doing. So I'll just check if this PR doesn't break anything for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 1a9aa8d
Guix builds (x86_64):
871cf387d5d60efc0ec9e50f975a9f44b2e2f9b7d92d1f2744affc3c8a5e1655 guix-build-1a9aa8d4eedf/output/arm64-apple-darwin/bitcoin-1a9aa8d4eedf-arm64-apple-darwin-unsigned.tar.gz
6a8de4ac9647549d146a53a9167b00f72ac09168939284b76fa9b6cf81595fea guix-build-1a9aa8d4eedf/output/arm64-apple-darwin/bitcoin-1a9aa8d4eedf-arm64-apple-darwin-unsigned.zip
6a00443340b2b8d13ed86a5d3947845ec2da20cfb0f0f80cce1434a0a4581557 guix-build-1a9aa8d4eedf/output/arm64-apple-darwin/bitcoin-1a9aa8d4eedf-arm64-apple-darwin.tar.gz
ec0320586c07bcc6b80505ed6b3de6307fd99d43b505919bb4f95ff27f1d8991 guix-build-1a9aa8d4eedf/output/dist-archive/bitcoin-1a9aa8d4eedf.tar.gz
14a0907bf9c91a2ca19b5a9b53071159be36d82d69c287ffd6a621695f4a637d guix-build-1a9aa8d4eedf/output/x86_64-apple-darwin/SHA256SUMS.part
862b59e90473c37d4f95d083fccf12416ddc92f9177a2ffb71180a11b3432d9b guix-build-1a9aa8d4eedf/output/x86_64-apple-darwin/bitcoin-1a9aa8d4eedf-x86_64-apple-darwin-unsigned.tar.gz
ddcb16882dde50880f9dffc6ac7ab5ac01b220bbedad4c16e9ff15dcf3b30cef guix-build-1a9aa8d4eedf/output/x86_64-apple-darwin/bitcoin-1a9aa8d4eedf-x86_64-apple-darwin-unsigned.zip
720a0689c916e878a5ff050b6183e993af751950b906593d4cbaf4b22a22466c guix-build-1a9aa8d4eedf/output/x86_64-apple-darwin/bitcoin-1a9aa8d4eedf-x86_64-apple-darwin.tar.gz
Apparently, CMake expects to find the From https://github.com/hebasto/bitcoin/actions/runs/8885213906/job/24396062348:
|
Ported to the CMake-based build system in hebasto#180. |
772769a fixup! cmake: Add `Maintenance` module (Hennadii Stepanov) fa36e33 fixup! cmake: Add platform-specific linker flags (Hennadii Stepanov) c6dc19f fixup! build: Generate `toolchain.cmake` in depends (Hennadii Stepanov) cddeb04 fixup! cmake: Add root `CMakeLists.txt` file (Hennadii Stepanov) Pull request description: This PR ports bitcoin#29890 and fixes cross-compiling for macOS been broken since the recent sync/rebase [PR](#179). ACKs for top commit: m3dwards: Giving a utACK 772769a Tree-SHA512: f0f10317b1fd5e46d1b7f340f4efb9f1b27a6a7a9b11191736c8edf32c278ba6d9ca4ef38d03a78cb40d58637e7e9746020b264a3f7eeb9903c23cb726f18fbf
Neither of these tools are actually used when we are cross-compiling for macOS. They are used when we have to adjust non-static libs during a deploy after building on a macOS machine. Simplies #29739 (will be rebased on top).
Guix (aarch64):